Classify provider quota errors as environment faults on opencode-http - #324
Classify provider quota errors as environment faults on opencode-http#324jackmcintyre wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
WalkthroughA shared ChangesEnvironment fault classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Session as Adapter session
participant Opencode as OpencodeHttpAdapter
participant ServerLog as task.server.out
participant Classifier as EnvFaultMixin
participant Result as SessionResult
Session->>Opencode: start or re-arm task
Opencode->>ServerLog: unlink stale server log
Opencode->>ServerLog: capture server output
Session->>Classifier: classify non-completed result
Classifier->>ServerLog: read and scan log tail
ServerLog-->>Classifier: matched fault evidence
Classifier->>Result: set env_fault and evidence
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
526006e to
9d0458b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_generic_tmux.py (1)
4345-4363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test passes with the straddler drop removed.
The 72 KB of padding pushes the matching line entirely outside the 64 KiB window, so nothing is scanned that could match — the assertion holds whether or not
lines[1:]runs. Sizing the padding just underENV_FAULT_TAIL_BYTESso the seek cuts through the matching line would actually pin the drop (and the U+FFFD-head concern the docstring cites).♻️ Sketch: put the straddler at the window edge
- straddler = b"API Error: Connection refused STRADDLER\n" - pad = b"filler line\n" * 6000 # comfortably over ENV_FAULT_TAIL_BYTES - _write_task_log(adapter, straddler + pad) + straddler = b"API Error: Connection refused STRADDLER\n" + # Land the seek INSIDE the straddler: everything after it is exactly + # ENV_FAULT_TAIL_BYTES minus half the line, so the window opens mid-match. + pad = b"filler line\n" * ((generic.ENV_FAULT_TAIL_BYTES - len(straddler) // 2) // 12) + _write_task_log(adapter, b"lead\n" + straddler + pad)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_generic_tmux.py` around lines 4345 - 4363, Update test_classify_env_fault_drops_the_partial_line_at_the_tail_seek so the matching straddler crosses the ENV_FAULT_TAIL_BYTES read boundary rather than being entirely before it. Size or position the padding to place the seek in the middle of straddler, then retain assertions that the partial matching line is discarded and no env-fault evidence is produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/adapters/opencode_http.py`:
- Around line 370-378: Update OpencodeHttpAdapter.start_session to unlink the
existing <task_id>.server.out before invoking _spawn_server, while preserving
the append behavior across port-collision retries within one spawn. Keep the
existing result.json cleanup and ensure the reset occurs at the start of each
new session cycle.
In `@tests/test_opencode_http.py`:
- Around line 2507-2510: Update the test docstring describing the fake server’s
output file to reference the asserted <task_id>.server.out filename instead of
logs/<task_id>.log. Keep the rest of the end-to-end session behavior description
unchanged.
---
Nitpick comments:
In `@tests/test_generic_tmux.py`:
- Around line 4345-4363: Update
test_classify_env_fault_drops_the_partial_line_at_the_tail_seek so the matching
straddler crosses the ENV_FAULT_TAIL_BYTES read boundary rather than being
entirely before it. Size or position the padding to place the seek in the middle
of straddler, then retain assertions that the partial matching line is discarded
and no env-fault evidence is produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68dafd8f-defd-47e3-9285-173499d50ee5
📒 Files selected for processing (10)
src/bmad_loop/adapters/base.pysrc/bmad_loop/adapters/env_fault.pysrc/bmad_loop/adapters/generic.pysrc/bmad_loop/adapters/opencode_http.pysrc/bmad_loop/adapters/profile.pysrc/bmad_loop/data/profiles/opencode.tomltests/test_env_fault_patterns.pytests/test_generic_tmux.pytests/test_opencode_http.pytests/test_profile.py
Greptile SummaryThis PR fixes a missing environment-fault classification on the
Confidence Score: 5/5Safe to merge — the change is additive (a new mixin + a new log unlink) with no modifications to the core run() control flow or result handling, and is guarded by a thorough test suite including an end-to-end regression through run(). The mixin's guard conditions (status membership, result_json check) are unchanged from the pre-existing GenericAdapter implementation and protect the rest of the result path. The stale-log unlink parallels the existing pattern in GenericAdapter.start_session and cannot corrupt a live session. The env_fault_patterns are narrowly anchored on a server-owned structured field and have been verified against a corpus of realistic bait lines. No regressions were found. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/adapters/env_fault.py | New EnvFaultMixin extracted from GenericAdapter; adds truncated-head fragment drop, both-end ellipsis markers, match-centred windowing, and ENV_FAULT_LOG_SUFFIX class variable so each transport scans the right file. |
| src/bmad_loop/adapters/opencode_http.py | Mixes in EnvFaultMixin, overrides ENV_FAULT_LOG_SUFFIX to ".server.out", and unlinks the server log at session start to prevent stale-log false positives on re-armed runs. |
| src/bmad_loop/adapters/generic.py | Removes the inline classify/evidence implementation (now in EnvFaultMixin) and re-exports the moved constants for backward compat; GenericAdapter now inherits from EnvFaultMixin. |
| src/bmad_loop/data/profiles/opencode.toml | Seeds two env_fault_patterns anchored on error.error="AI_APICallError: …" — one for quota/rate-limit refusals, one for socket-drop connection failures — verified against a captured 5-hour outage. |
| tests/test_env_fault_patterns.py | New corpus-driven test file covering false-positive (BAIT), anchor-reaching bait, verbatim-citation characterisation, false-negative (REAL_BY_PROFILE), and unseeded-profile inertness; also pins claude's known false-positive as xfail(strict). |
| tests/test_opencode_http.py | Adds eight new env-fault tests including an end-to-end run() regression that pins the scan to .server.out rather than .log, and a stale-log unlink test that drives a real session. |
| tests/test_generic_tmux.py | Adds three new tests: dropped-suffix ellipsis, partial-line fragment drop on tail seek, and no-newline degenerate window; also hardens the inert-without-patterns test to build an empty profile rather than borrow a shipped profile. |
Reviews (3): Last reviewed commit: "fix(adapters,profiles): classify provide..." | Re-trigger Greptile
9d0458b to
9c6a3d6
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_generic_tmux.py`:
- Around line 4351-4355: Adjust the test data setup around straddler and pad so
the 64 KiB tail-read boundary falls within the straddler line itself, rather
than after it in the padding. Preserve the scenario where no other matching line
exists, ensuring the test fails if partial lines at the tail boundary are
scanned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: beb32dbc-4822-4ed3-aa68-9292eed2cbd3
📒 Files selected for processing (10)
src/bmad_loop/adapters/base.pysrc/bmad_loop/adapters/env_fault.pysrc/bmad_loop/adapters/generic.pysrc/bmad_loop/adapters/opencode_http.pysrc/bmad_loop/adapters/profile.pysrc/bmad_loop/data/profiles/opencode.tomltests/test_env_fault_patterns.pytests/test_generic_tmux.pytests/test_opencode_http.pytests/test_profile.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/bmad_loop/adapters/profile.py
- src/bmad_loop/data/profiles/opencode.toml
- tests/test_profile.py
- tests/test_opencode_http.py
- src/bmad_loop/adapters/base.py
- src/bmad_loop/adapters/generic.py
…encode-http A hard provider usage-limit error was not recognised as an environment fault. The loop recorded env_fault=false, spent max_dev_attempts on the story, deferred it, and moved to the next story — which hit the same wall. One 5-hour quota window became three deferred stories and seven dead sessions, four of which consumed zero tokens. STRUCTURAL FIX. run() has always called _classify_env_fault for every adapter (adapters/base.py), but the implementation lived on GenericAdapter while OpencodeHttpAdapter is its sibling, not its subclass — so the base identity no-op applied and the feature was absent. Its docstring justified that with "adapters with no post-mortem signal (HTTP/mock)", a premise that stopped being true once opencode_http began teeing the serve process's stdout/stderr to logs/<task_id>.log — the exact path the classifier reads. Hoist the classifier into adapters/env_fault.EnvFaultMixin and mix it into both adapters. The signal is the log, not the transport, so any adapter writing logs/<task_id>.log inherits it rather than re-omitting it. Patterns compile lazily off self.profile, so mixing the class in is the whole wiring step — there is no __init__ line to forget, which is how this was missed. WHICH FILE IS SCANNED IS PART OF THE CONTRACT. EnvFaultMixin gained ENV_FAULT_LOG_SUFFIX; opencode overrides it to ".server.out". Its "<task_id>.log" is the curated [bmad] conversation transcript written by the SSE reader, so it carries the model's own words, while the provider's AI_APICallError logfmt lines only ever land in the server sink. Scanning the transcript would break two things at once: the evidence would not be there, and the profile's patterns — which are only sound against a model-free log — would start matching stories that quote a provider error. Unit tests could not catch this (they write their fixture to whatever path the code resolves); only the end-to-end test through run() did. The scanned file is also DROPPED at start_session, mirroring what GenericAdapter already does with its pane tee. A re-armed run reuses task_ids and the server sink is opened "ab", so without this the next session would scan the PREVIOUS cycle's provider error — and this bites hardest on the exact path the classifier serves: env fault pauses the run, the operator re-arms, the next session rescans the stale refusal and pauses again, however healthy its own log. A pause loop surviving every re-arm, off one stale line. PATTERNS: ONE PROFILE, DELIBERATELY. opencode.toml is seeded, anchored on the structured field the serve process emits (error.error="AI_APICallError: …") and verified by replaying the shipped profile through the mixin over a real outage's logs: four sessions classify, the healthy merged story's do not, and the first hit lands on story 2's second attempt — where the run now pauses instead of marching into stories 3, 4 and 5. The gap inside that pattern is [^"], not `.`, so it cannot walk past the closing quote and find a cause word in a later logfmt field — otherwise an unrelated `AI_APICallError: Invalid API key` on a line whose trailing fields mention a quota path reads as a quota outage. codex, gemini, copilot and antigravity stay INERT. Patterns for them were drafted and withdrawn. They could only be written from error strings scraped off public issue trackers, for CLIs nobody here has run, and an unverified pattern is not a neutral bet: the tmux adapters match against a pane capture containing the model's own output, so a pattern that fires on a story which merely *implements* rate limiting pauses a healthy run. That inverse bug is worse than the miss, because the miss only reproduces today's behaviour. An adversarial pass demonstrated it on realistic fixture and assertion text before these shipped. A quota pattern was drafted for claude.toml and withdrawn for the same reason, so that profile is untouched from main: an Anthropic plan's usage limit is still unclassified. Its existing connection pattern already matches ordinary prose about provider errors — including this repo's own CHANGELOG and docs — which is pre-existing bmad-code-org#194 debt, now pinned by an xfail(strict) test rather than left undocumented. ALSO FIXED in the classifier, both found by adversarial review: - The 64 KiB tail seek lands on an arbitrary byte, so the line straddling the window edge arrived as a fragment whose head could be a U+FFFD from the errors="replace" decode. That fragment is now discarded rather than matched and quoted as evidence. - _excerpt marked a dropped prefix but silently dropped a suffix, so a truncated excerpt read as a complete line that simply ended there. Both ends are marked now, and the markers are spent FROM the evidence budget rather than added on top of it. - The evidence window is centred on the match rather than the line head: an opencode logfmt line carries ~250 characters of metadata before error.error=, so the operator previously saw every field except the failure. Test corpora are split into the half that must classify (captured error lines), the half that must not (realistic healthy-session output), and a third that pins the LIMIT of content-based matching: a story quoting a provider error verbatim produces a line byte-identical to the emitted one, so it matches and no pattern can separate them. That is acceptable here only because opencode's scanned log is the serve process's own stdout, which the model cannot write to; it is asserted positively so it cannot change silently. Reachability of the bait is itself asserted — this corpus was briefly vacuous, guarding nothing while looking thorough, after the profile it had been written for was withdrawn. Correlation ids in the test corpora are fakes; the run they were captured from is a private project. Claude-Session: https://claude.ai/code/session_01TXq8NfCFZv922RYqx1kU3s
9c6a3d6 to
64efb7e
Compare
Addresses #323. Doesn't close it: the four unseeded profiles and the zero-token timeout heuristic are still open, and so is the claude gap described below.
A provider usage-limit error wasn't recognised as an environment fault. The loop recorded
env_fault: false, spentmax_dev_attemptson the story, deferred it, and moved to the next story, which hit the same wall. One 5-hour quota window turned into three deferred stories and seven dead sessions, four of which consumed zero tokens. The specs were fine. The first story of the run completed and merged on the same adapter and model, it just happened to be the one that exhausted the quota.The structural half
run()has always called_classify_env_faultfor every adapter. The implementation lived onGenericAdapter, andOpencodeHttpAdapteris its sibling rather than its subclass, so the base identity no-op applied and the feature was absent. The base docstring justified that with "adapters with no post-mortem signal (HTTP/mock)", which stopped being true onceopencode_httpstarted teeing the serve process's stdout tologs/<task_id>.log. That's the exact file the classifier reads.The classifier now lives in
adapters/env_fault.EnvFaultMixinand both adapters mix it in. The signal is the log, not the transport, so any adapter that writeslogs/<task_id>.loginherits it instead of re-omitting it. Patterns compile lazily offself.profile, so mixing the class in is the whole wiring step. There's no__init__line to forget, which is how this got missed the first time.One profile seeds patterns, on purpose
opencode.tomlis anchored on the structured field the serve process emits,error.error="AI_APICallError: ...", and verified by replaying the shipped profile through the mixin over a real outage's logs. Four sessions classify, the healthy merged story's three don't, and the first hit lands on story 2's second attempt. That's where the run now pauses instead of marching into stories 3, 4 and 5.codex,gemini,copilotandantigravitystay inert. I drafted patterns for all four and withdrew them. They could only be written from error strings scraped off public issue trackers, for CLIs I haven't run, and an unverified pattern isn't a neutral bet. The tmux adapters match against a pane capture that contains the model's own output, so a pattern that fires on a story which merely implements rate limiting pauses a healthy run. That's worse than the miss, because the miss only reproduces current behaviour. An adversarial pass demonstrated it on realistic fixture and assertion text before any of them shipped.I also drafted a quota pattern for
claude.tomland withdrew it for the same reason, so that profile is untouched. An Anthropic plan's usage limit is still unclassified on that adapter. Its existing connection pattern already matches ordinary prose about provider errors, including this repo's own CHANGELOG and docs, so a bmad-loop session working on bmad-loop is exposed. That's pre-existing #194 debt. It's now pinned by anxfail(strict)test rather than left undocumented, and fixing it needs a captured Claude Code quota line plus a stronger anchor than "API Error".Which file gets scanned is part of the contract
Rebasing onto current main turned up a fourth defect, and it's the one worth reading.
The readable-logs work repurposed opencode's
logs/<task_id>.log. It's now the curated[bmad]conversation transcript written by the SSE reader, and the server's own stdout moved to<task_id>.server.out. The classifier was still scanning.log, which breaks two things at once. The provider'sAI_APICallErrorlines aren't there any more, so nothing would ever classify. And the patterns are only sound against a log the model can't write to, so pointing them at a transcript of the model talking would have made the verbatim-citation false positives live.EnvFaultMixinnow hasENV_FAULT_LOG_SUFFIX, and the opencode adapter overrides it to.server.out. The safety property is documented at the override, because it's a property of the file, not of the patterns.None of the unit tests caught this. They write their fixture to whatever path the code resolves, so they agree with any answer. The only thing that caught it was the end-to-end test through
run(), which exists because a reviewer flagged its absence as the lowest-severity finding in the whole review.Also fixed in the classifier
Three more defects, all found by adversarial review rather than by me.
The 64 KiB tail seek lands on an arbitrary byte, so the line straddling the window edge arrived as a fragment whose head could be a
U+FFFDfrom theerrors="replace"decode. That fragment is discarded now, unless discarding it would leave nothing to scan._excerptmarked a dropped prefix but silently dropped a suffix, so a truncated excerpt read as a complete line that ended there. Both ends are marked, and the markers are spent from the evidence budget rather than added on top of it.The evidence window is centred on the match and slides left when the match sits near the end of the line. opencode's logfmt puts around 250 characters of metadata before
error.error=, so before this the operator saw every field except the failure. The quota evidence now reads through to the reset timestamp.Tests
Corpora are split three ways: lines that must classify (captured from the outage), lines that must not (realistic healthy-session output), and lines that pin the limit of the approach. A story quoting a provider error verbatim produces a line byte-identical to the emitted one, so it matches, and no content-based pattern can separate them. That's tolerable for opencode only because the file it scans is
<task_id>.server.out, which the model can't write to. It's asserted positively so it can't change quietly, and that assertion is the one that will fire if the scanned file is ever repointed at something carrying model output again.Reachability of the bait is asserted too. That corpus was briefly vacuous, guarding nothing while looking thorough, after the profile it had been written for was withdrawn.
Correlation ids in the corpora are fakes. The run they came from is a private project.
Verification
3323 passed, 42 skipped, 5 xfailed. The xfails are all the claude debt test.
ruff,black,isortandpyrightclean.The replay over the original outage logs was re-run after the rebase, with the log suffix overridden, because that run predates the
.log/.server.outsplit and its captured bytes were written when.logwas still the server sink. Same four sessions classify, the healthy story's three still don't.Summary by CodeRabbit